Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 654 - gzipping asset bundles #656

Merged
merged 14 commits into from
Jan 15, 2024
Merged

Issue 654 - gzipping asset bundles #656

merged 14 commits into from
Jan 15, 2024

Conversation

sebjf
Copy link
Contributor

@sebjf sebjf commented Dec 6, 2023

This fixes #654

Description

This PR gzips the Unity Asset Bundles by default, and provides a metadata BSON so that the state of this encoding is stored in the db.

This PR exists for now just for feedback.

@sebjf
Copy link
Contributor Author

sebjf commented Dec 12, 2023

Hi @Charence, the build failed on this branch previously. I have started it again but both times I cannot see its progress:

image

Do you know what's up with it?

@carmenfan carmenfan self-assigned this Dec 13, 2023
@carmenfan carmenfan self-requested a review December 13, 2023 09:59
@carmenfan carmenfan changed the title Issue 654 Issue 654 - gzipping asset bundles Dec 13, 2023
in.push(inputStream);
boost::iostreams::copy(in, ss);

repo::core::model::RepoBSONBuilder builder;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebjf

I wouldn't do it this way. This is quite difficult to read and making the way we store metadata data about a file highly coupled with the functions/a classes that are simply passing in the files.

I think this could be a lot simpler (Unless i'm missing something, which I'm often am!). I think the file manager can manage the gzipping and it should just be a flag on the function https://github.com/3drepo/3drepobouncer/blob/master/bouncer/src/repo/core/handler/fileservice/repo_file_manager.h#L60

If this is set to true, it would know to append the gzip flag onto the metadata of the file ref.

We could just gzip all the geo files we export, or it could be a flag in the exporter to indicate whether we want to gzip this. (Persumably we benefit from gzipping the SRCs too, but I'm not sure how unreal handles those requests!)

And it would also make gzipping the JSON caches much easier to do if we want to implement it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @carmenfan, this should be resolved now!

… to control it. Set all the webbuffers to be gzipped by default, because .io can decompress as required.
@carmenfan carmenfan self-requested a review December 14, 2023 11:17
Copy link
Member

@carmenfan carmenfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@sebjf
This runs fine, works fine, but failing to build in linux

@sebjf
Copy link
Contributor Author

sebjf commented Jan 9, 2024

@carmenfan, this should be fixed now for when Travis comes back!

@sebjf
Copy link
Contributor Author

sebjf commented Jan 11, 2024

/azp run

Copy link

No pipelines are associated with this pull request.

@carmenfan carmenfan self-requested a review January 11, 2024 16:12
@carmenfan carmenfan merged commit 1f9b1da into staging Jan 15, 2024
6 of 7 checks passed
@carmenfan carmenfan removed their assignment Jan 16, 2024
@carmenfan carmenfan deleted the ISSUE_654 branch March 6, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants